Add visitor for converting kernel expressions to engine expressions #363

OussamaSaoudi-db · 2024-09-27T20:55:10Z

Rationale for the change

Engines may need to apply expressions that originate in the kernel. This can include filter predicates such as filters for protocol and metadata actions in a log file. Another example is logical to physical transformations that the engine must apply, for instance to materialize partition columns. This PR introduces a visitor framework for kernel expressions to be converted to engine expressions so that engines can apply their own evaluators, predicate pushdowns, and optimizations on these expressions.

Closes: #358

What changes are included in this PR?

5 major changes are made in this PR:

A new module ffi/src/expressions/kernel.rs is added. This module is responsible for converting kernel Expressions to the engine's representation of expressions through an ffi-compatible visitor. The visitor supports binary operations, scalars, struct expressions, variadic and unary operators, and columns. All the engine has to do is implement an EngineExpressionVisitor to support the conversion.
The EnginePredicate to kernel Expressionvisitor code is moved to ffi/src/expressions/engine.rs. This is done to keep all ffi expression code under the ffi/src/expressions module.
This PR also allows engines to get SharedExpressions, which are handles to the kernel's Expression type. These are given back to the kernel to specify which expression the engine would like to visit.
A new ffi example is added to showcase how the EngineExpressionVisitor can be used to construct expressions. This is a C implementation that can be found in ffi/examples/visit-expression/. The example only prints an expression that it receives from the kernel and asserts that it matches an expected output.
A new testing feature flag test-ffi is added to the ffi crate. This feature flag is used to expose testing functions found in ffi/src/test_ffi. This module is currently used to return a kernel expression get_testing_kernel_expression used in the example c implementation.

Are these changes tested?

Two tests are added:

I introduced a new test to validate the expression visitor functions correctly and converts data as expected. This simply prints out the expression tree structure and compares it to an expected result. The kernel expression can be found in ffi/src/test_ffi.rs, and the expected output is in ffi/tests/test-expression-visitor/expected.txt
There is also a test that checks for memory leaks in the expression visitor using valgrind.

Are there any user-facing changes?

This PR introduces a public facing API for constructing expressions and visiting them. It also adds delta_kernel_derive as a public export of kernel crate. This PR does not break existing APIs.

codecov · 2024-09-27T20:58:31Z

Codecov Report

Attention: Patch coverage is 2.07469% with 236 lines in your changes missing coverage. Please review.

Project coverage is 77.25%. Comparing base (f5d0a42) to head (60049ab).

Files with missing lines	Patch %	Lines
ffi/src/expressions/kernel.rs	0.00%	160 Missing ⚠️
ffi/src/test_ffi.rs	0.00%	76 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #363      +/-   ##
==========================================
- Coverage   78.94%   77.25%   -1.70%     
==========================================
  Files          52       54       +2     
  Lines       10756    10994     +238     
  Branches    10756    10994     +238     
==========================================
+ Hits         8491     8493       +2     
- Misses       1796     2032     +236     
  Partials      469      469

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ffi/src/expressions.rs

FFI code for schema and expressions have been moved to their own modules. Add prototype of the visitor to enable kernel expresison -> engine expression

nicklan

Nice! Pretty much there, just a couple of small things.

nicklan · 2024-10-23T21:00:35Z

ffi/src/expressions/kernel_expr_visitor.rs

+        }
+        call!(visitor, visit_array_literal, sibling_list_id, child_list_id);
+    }
+    fn visit_struct_literal(


hrmm, this is a tricky one. I don't love converting the field names into string scalars and maintaining two lists.

We already have a way of doing iterators, so what if we visit all the values, and then visit_struct_literal could take an iterator over KernelStringSlice that will return field names, and the list id where the engine can find the value for each field.

I don't love that solution either, but it feels more clean than the engine having to unwrap all these string scalars into just strings. The downside is that you're pairing an iterator to a list, so the engine has to deal with potentially two different ways of iterating.

I realize this might all change since we're potentially doing away with/seriously changing StructData, so if you think it's not worth tackling until then I'm okay with this in the short term.

also aside: can we rename this internal function so it doesn't have the same name as the visitor one, it's confusing :)

This was intended as a quick placeholder from the beginning because StructField would've been better to pass. On top of that we're planning a major rework of scalars/structs/arrays, so I think this is okay as a first draft.

Renamed kernel visitor functions to visit_expression_* 👍

kernel/src/expressions/scalars.rs

ffi/tests/test_expression_visitor/run_test.sh

nicklan · 2024-10-23T21:12:42Z

Also, I think the coverage is listed as so low here because the testing happens in c. Could we update our codecov runner to somehow also capture that coverage? cc @zachschuermann

Fine to take that as a follow-up. Please create an issue if that's the plan.

zachschuermann · 2024-10-23T21:16:05Z

Also, I think the coverage is listed as so low here because the testing happens in c. Could we update our codecov runner to somehow also capture that coverage? cc @zachschuermann

Fine to take that as a follow-up. Please create an issue if that's the plan.

Yup looks like it - created #425

zachschuermann

ok whew that was a lot haha - haven't reviewed C in a while.. LGTM but left a few nits/follow-ups/etc.

ffi/src/test_ffi.rs

zachschuermann · 2024-10-21T22:30:31Z

.github/workflows/build.yml

        run: |
          pushd ffi/examples/read-table
          mkdir build
          pushd build
          cmake ..
          make
          make test
+      - name: build and run visit-expression test


does this run in parallel with the first (low prio)?

Nope, this is sequential.

ffi/src/expressions/kernel_expr_visitor.rs

ffi/examples/visit-expression/CMakeLists.txt

ffi/examples/visit-expression/expression.h

zachschuermann · 2024-10-24T16:09:09Z

ffi/examples/visit-expression/expression.h

+}
+
+void free_expression_list(ExpressionItemList list);
+void free_expression_item(ExpressionItem ref) {


I remember you said you were battling memory leak - did you valgrind to validate it now? Wonder if we should include valgrind tests somehow (maybe just do fastest/easiest thing for now - like docs that say "I ran valgrind with blah blah)

Yeah I considered that. There's some platform stuff that I'd have to do I think. For example, macos doesn't have valgrind, it uses leaks. Linux has valgrind. I'll give it a shot to run it through valgrind and leaks depending on os.

Update: Added a linux-only valgrind test. Macos (and i think windows) don't have valgrind, and I didn't think the cross platform test is necessary.

yea just linux I think would certainly suffice :)

ffi/src/expressions/kernel_expr_visitor.rs

zachschuermann · 2024-10-24T16:24:33Z

ffi/src/lib.rs

+#[cfg(feature = "test-ffi")]
+pub mod test_ffi;


also instead of making new feature flag we could just put behind usual test cfg?

I looked into this. There's no easy way to do this as far as I know. cbindgen ignores all code under #[cfg(test)].

One idea is to have a separate build.rs that's exclusively for testing. This can include a test crate using the bulider.

Either way we'll have to rethink how we handle our current ffi tests imo

zachschuermann · 2024-10-24T16:33:22Z

Also @OussamaSaoudi-db can we update the PR description? There are a number of changes here and I think it would help if we have a PR desc that says something to the effect of a list (1) moves old expression visitor to engine_expr_visitor (2) adds kernel_expr_visitor mod which defines EngineExpressionVisitor (3) new expression visitor example implementation in C in ffi/examples/visit-expression/ (4) etc. etc.

scovich

Looking really good. A few items to address before merge.

ffi/examples/read-table/expression.h

kernel/src/schema.rs

ffi/src/test_ffi.rs

scovich · 2024-10-16T19:20:16Z

kernel/src/expressions/scalars.rs

@@ -20,7 +20,8 @@ pub struct ArrayData {
 }

 impl ArrayData {
-    pub fn new(tpe: ArrayType, elements: Vec<Scalar>) -> Self {
+    pub fn new(tpe: ArrayType, elements: impl IntoIterator<Item = impl Into<Scalar>>) -> Self {


I've been thinking about this idiom, and how it gets annoying for passing empty lists (see above, where vec![] no longer compiles and we have to instead do Vec::<Scalar>::new().

I couldn't find a way to infer a "reasonable" type from [] or vec![], so the only other option is to define an empty method. But that's not great, either.

In the end, it may not matter much, because these new-empty cases are almost entirely confined to test code. But on the other hand most expression-using code is actually test code, and there's an argument for making tests easier to write and maintain.

Yeah it's definitely annoying 😔 I personally want to see macros to easily put together expressions, struct data, etc. Perhaps such a macro could handle this case. but we're a long way from that point.

ffi/src/test_ffi.rs

scovich · 2024-10-24T21:44:49Z

ffi/src/expressions/kernel.rs

+/// ([`usize`]) the engine wants, and will be passed back to the engine to identify the list in the
+/// future.
+///
+/// Every expression the kernel visits belongs to some list of "sibling" elements. The schema


aside: "sibling" has always seemed a confusing/unhelpful name to me (which is bad, given that I named it). Maybe it's better termed an "owner" or "parent" list? (as in, the list that owns me -- different from the child list that I own)?

Open to other ideas, and certainly doesn't need to be addressed in this PR.

Yeah it took me a while to understand it. I think parent and owner might help.

Bit of a curiosity question: Any prior work on this style of visitor pattern? I've seen examples of visitor pattern where we simply get back the item. Here we're providing a list to put the item into.

I have not seen this "double trampoline" pattern previously. It is motivated by kernel's particular constraints when it comes to memory management (engine and kernel have separate memory and they don't mix nicely).

ffi/src/expressions/kernel.rs

ffi/src/test_ffi.rs

OussamaSaoudi-db · 2024-10-24T23:57:36Z

kernel/src/lib.rs

@@ -75,6 +75,7 @@ pub mod snapshot;
 pub mod table;
 pub(crate) mod utils;

+pub use delta_kernel_derive;


@scovich I wanted to flag this. I had to do this to do this because the column_name macro (used by column_expr) uses the macro from delta_kernel_derive.

It seems ok? But I'll admit I don't know the implications of making that crate public?

it's a separate crate so I think we can tackle this two ways?

(this approach) we can export it via kernel since it's a dependency of kernel or

we can just import it as a separate crate into FFI crate right?

seems like better separation of concerns to just to (2) but since kernel will always depend on delta_kernel_derive, I could also be okay with option (1).

curious to hear from @nicklan

It seems ok? But I'll admit I don't know the implications of making that crate public?

AFAIK the crate is already public, and this is just re-exporting it as a public sub-crate from kernel.

Laying out the 2 options was rly helpful!

It feels like it also makes sense to do 1 from a usability standpoint. Suppose delta-rs wants to create columns using column_expr! or column_name!. Without the re-export, they'd get a compilation error coming from the macro, and they'd have to go through a couple layers of macro before finding why it fails. This was my experience using the macros

OussamaSaoudi-db · 2024-10-25T00:38:22Z

Posting for some visibility: We only do memory leak testing for linux. This is because macos does not support valgrind. It has leaks, but it somehow doesn't play nicely with github actions and times out. Similar problem encountered by others.

I think it's sufficient to just test memleaks on linux.

OussamaSaoudi-db · 2024-10-25T01:09:22Z

ffi/examples/visit-expression/CMakeLists.txt

+target_compile_options(visit_expression PRIVATE -Wall -Wextra -Wpedantic -Werror -Wno-strict-prototypes)
+
+# Get info on the OS and platform
+if (${CMAKE_SYSTEM_NAME} MATCHES "Darwin")


@zachschuermann just wanted to have a second look. Slightly yucky, but it's the way I found to target linux for valgrind. Note that macos does not support valgrind.

scovich · 2024-10-28T13:16:48Z

Posting for some visibility: We only do memory leak testing for linux. This is because macos does not support valgrind. It has leaks, but it somehow doesn't play nicely with github actions and times out. Similar problem encountered by others.

I think it's sufficient to just test memleaks on linux.

That seems reasonable -- I don't see why leak behavior should be platform-dependent. A leak is a leak is a leak, most likely.

nicklan

lgtm! thanks

scovich reviewed Sep 28, 2024

View reviewed changes

ffi/src/expressions.rs Outdated Show resolved Hide resolved

OussamaSaoudi-db added 9 commits October 2, 2024 14:56

Move ffi code, add kernel expr -> engine expr

6e1bbf1

FFI code for schema and expressions have been moved to their own modules. Add prototype of the visitor to enable kernel expresison -> engine expression

Remove new visitor

b367df1

Add kernel expression to engine expression

1bc700a

Add ffi validation and more visitor features

dc1ed23

initial type expansion

8ce6338

Add new impls for visitor

384056b

Add even more expression visitor stuff

10b6534

Patch expr_struct

e7aa687

Fix ffi test

57fdf77

OussamaSaoudi-db force-pushed the expresions_visitor2 branch from 1814c84 to 57fdf77 Compare October 2, 2024 21:56

OussamaSaoudi-db added 7 commits October 2, 2024 15:05

Fix clippy issues

27736a0

Add support for structs, arrays, null

5ff4396

Improve literal type system

bacc7b2

Remove cache files

318453e

Add Remaining expression visitor functionality

5217dc6

Add deconstructor and make functions consistently named

778c84c

More fixes to expression

07c458b

github-actions bot added the breaking-change Change that will require a version bump label Oct 9, 2024

OussamaSaoudi-db added 8 commits October 9, 2024 16:36

Fix all memory leaks

23d1ed2

Merge branch 'main' into expresions_visitor2

ccf47b2

Remove clang cache

c66fb3e

Fix safety, remove unnecessary diffs

45c2a99

Add docs to visit_expression

097fe8b

Add expressions test

4b0ab65

Add docs, improve style

1af2939

Fix spacing

4df19e3

OussamaSaoudi-db changed the title ~~[WIP] Add visitor for converting kernel expressions to engine expressions~~ Add visitor for converting kernel expressions to engine expressions Oct 10, 2024

OussamaSaoudi-db requested a review from nicklan October 10, 2024 22:03

nicklan requested changes Oct 23, 2024

View reviewed changes

zachschuermann mentioned this pull request Oct 23, 2024

make sure codecov includes c tests #425

Open

zachschuermann approved these changes Oct 24, 2024

View reviewed changes

OussamaSaoudi-db added 6 commits October 24, 2024 10:43

Rename the expressions modules

bdd28d7

Apply review comments

14a5c15

Address comments

c22e311

Try windows without MSVC

49f6198

Fix wording

633a4ba

Add dev visibility

4faff29

scovich approved these changes Oct 24, 2024

View reviewed changes

OussamaSaoudi-db added 5 commits October 24, 2024 15:46

Flushing changes

984b615

Address pr comments

8530b8d

Merge branch 'main' into expresions_visitor2

306c982

Fix rebase issue

bbac1bb

hopefully fix linux leak test

00d0354

OussamaSaoudi-db commented Oct 24, 2024

View reviewed changes

Remove failing test

d282063

Renaming

d0218fd

OussamaSaoudi-db requested a review from nicklan October 25, 2024 01:06

Small nit

60049ab

OussamaSaoudi-db commented Oct 25, 2024

View reviewed changes

Merge branch 'main' into expresions_visitor2

abcf590

nicklan approved these changes Oct 28, 2024

View reviewed changes

OussamaSaoudi-db merged commit 5e5c299 into delta-incubator:main Oct 28, 2024
17 checks passed

OussamaSaoudi-db deleted the expresions_visitor2 branch October 28, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add visitor for converting kernel expressions to engine expressions #363

Add visitor for converting kernel expressions to engine expressions #363

OussamaSaoudi-db commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

nicklan left a comment

nicklan Oct 23, 2024

OussamaSaoudi-db Oct 24, 2024

OussamaSaoudi-db Oct 24, 2024

nicklan commented Oct 23, 2024

zachschuermann commented Oct 23, 2024

zachschuermann left a comment

zachschuermann Oct 21, 2024

OussamaSaoudi-db Oct 24, 2024

zachschuermann Oct 24, 2024

OussamaSaoudi-db Oct 24, 2024

OussamaSaoudi-db Oct 25, 2024

zachschuermann Oct 25, 2024

zachschuermann Oct 24, 2024

OussamaSaoudi-db Oct 24, 2024

zachschuermann commented Oct 24, 2024

scovich left a comment

scovich Oct 16, 2024

OussamaSaoudi-db Oct 25, 2024

scovich Oct 24, 2024

OussamaSaoudi-db Oct 24, 2024

scovich Oct 28, 2024

OussamaSaoudi-db Oct 24, 2024

scovich Oct 25, 2024

zachschuermann Oct 25, 2024

zachschuermann Oct 25, 2024

OussamaSaoudi-db Oct 25, 2024 •

edited

Loading

OussamaSaoudi-db commented Oct 25, 2024

OussamaSaoudi-db Oct 25, 2024

scovich commented Oct 28, 2024

nicklan left a comment

Add visitor for converting kernel expressions to engine expressions #363

Add visitor for converting kernel expressions to engine expressions #363

Conversation

OussamaSaoudi-db commented Sep 27, 2024 • edited Loading

Rationale for the change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

codecov bot commented Sep 27, 2024 • edited Loading

Codecov Report

nicklan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicklan commented Oct 23, 2024

zachschuermann commented Oct 23, 2024

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachschuermann commented Oct 24, 2024

scovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OussamaSaoudi-db Oct 25, 2024 • edited Loading

Choose a reason for hiding this comment

OussamaSaoudi-db commented Oct 25, 2024

Choose a reason for hiding this comment

scovich commented Oct 28, 2024

nicklan left a comment

Choose a reason for hiding this comment

OussamaSaoudi-db commented Sep 27, 2024 •

edited

Loading

codecov bot commented Sep 27, 2024 •

edited

Loading

OussamaSaoudi-db Oct 25, 2024 •

edited

Loading